Skip to content

cpu/sam0_common: derive ROM_LEN & RAM_LEN from part number#13607

Merged
dylad merged 1 commit intoRIOT-OS:masterfrom
benpicco:cpu/sam0_common/generic_RAM_ROM
Mar 31, 2020
Merged

cpu/sam0_common: derive ROM_LEN & RAM_LEN from part number#13607
dylad merged 1 commit intoRIOT-OS:masterfrom
benpicco:cpu/sam0_common/generic_RAM_ROM

Conversation

@benpicco
Copy link
Contributor

Contribution description

The ROM size is encoded in the part number of the Atmel SAM chips.
RAM size is not encoded directly, but it's always a fixed fraction of the ROM size.

  • On SAM D2x/SAML2x it's ⅛ of the ROM.
  • On SAM L1x it's ¼ of the ROM.
  • On SAM D5x RAM is at least 128k and increments by 64k with every ROM increment.

Testing procedure

I manually confirmed that the resulting ROM_LEN and RAM_LENs are still the same for the chips that were listed here before.

Issues/PRs references

This is the last missing piece missing to do away with having to do changes in RIOT for using a different part number of a supported chip.

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: cpu Area: CPU/MCU ports labels Mar 10, 2020
@benpicco benpicco requested a review from dylad March 10, 2020 14:02
@benpicco benpicco force-pushed the cpu/sam0_common/generic_RAM_ROM branch from dba83fa to bc84822 Compare March 10, 2020 14:05
@dylad
Copy link
Member

dylad commented Mar 10, 2020

@benpicco:
Isn't easier to derive these values from vendor files ? using HSRAM_SIZE and FLASH_SIZE ?
No more maths, no more headache.

@benpicco
Copy link
Contributor Author

I'm afraid those are not available to make.
And then those variables are passed on to the linker which also doesn't understand header files.

@dylad
Copy link
Member

dylad commented Mar 10, 2020

I'm afraid those are not available to make.
And then those variables are passed on to the linker which also doesn't understand header files.

Yeah but I was thinking of using sed to parse the right file from the Makefile. CPU model should be known so It might be doable ? This way, we just have to add vendor files for new MCUs and Makefile will adapt automatically.

@benpicco
Copy link
Contributor Author

Yeah but I was thinking of using sed to parse the right file from the Makefile. CPU model should be known so It might be doable ? This way, we just have to add vendor files for new MCUs and Makefile will adapt automatically.

Good idea!
Unfortunately the sub-folder structure is not uniform, so I have to resort to find, but knowing the family folder already, this should not be too bad.

@dylad
Copy link
Member

dylad commented Mar 10, 2020

Good idea!
Unfortunately the sub-folder structure is not uniform, so I have to resort to find, but knowing the family folder already, this should not be too bad.

Nice, this looks cleaner this way and even more generic ! Let's wait for Murdock.

@fjmolinas
Copy link
Contributor

Good idea!
Unfortunately the sub-folder structure is not uniform, so I have to resort to find, but knowing the family folder already, this should not be too bad.

This could be very bad performance wise, it could lead to very big build times. In general anything that uses non make builtins is slow, so any shell call is slow. Parsing files can be very slow and IMO should be avoided, see #13174 for an example.

It is in general better to have a big file declaring everything (in make) than using magic parsing to retrieve that.

@benpicco
Copy link
Contributor Author

I reverted the last change, it was broken anyway according to Murdock.

@dylad
Copy link
Member

dylad commented Mar 10, 2020

see #13174 for an example.

I understand it may be faster for make but this is hell to maintain.

@benpicco
Copy link
Contributor Author

benpicco commented Mar 10, 2020

Not sure why ?= breaks riotboot, but there is no need to overwrite those constants, so we might just as well use :=.

Parsing the part number with sed should be uncontroversial. (Unless there is a way to do that with built-in commands)
I agree that using find to search for the right vendor file (because some of these have multiple include_[a|b|c|d]/ directories) and then parsing the huge vendor file might be sub-optimal.

# get vendor file to extract RAM size
VENDOR_FILE := $(shell find $(RIOTCPU)/sam0_common/include/vendor/sam$(FAMILY) -name $(CPU_MODEL).h | grep include*/sam)
RAM_LEN := $(shell sed -E -n 's/\#define (HMCRAMC0_SIZE|HSRAM_SIZE).*\((.*)\).*/\2/p' $(VENDOR_FILE))

But then again the compiler will do just the same thing and the file will be in the cache.

@benpicco benpicco force-pushed the cpu/sam0_common/generic_RAM_ROM branch from ec9fe67 to 51af582 Compare March 13, 2020 15:08
@benpicco
Copy link
Contributor Author

benpicco commented Mar 14, 2020

Why speculate when we can measure it?
I've run hyperfine on the different versions:

master

Benchmark #1: make BOARD=samr21-xpro -j
  Time (mean ± σ):     527.9 ms ±   4.9 ms    [User: 503.1 ms, System: 69.6 ms]
  Range (min … max):   519.7 ms … 537.2 ms    10 runs

semi-generic version with per-family formula

Benchmark #1: make BOARD=samr21-xpro -j
  Time (mean ± σ):     531.7 ms ±   2.1 ms    [User: 502.1 ms, System: 75.7 ms]
  Range (min … max):   528.6 ms … 536.2 ms    10 runs

generic version parsing vendor files

Benchmark #1: make BOARD=samr21-xpro -j
  Time (mean ± σ):     535.6 ms ±   4.0 ms    [User: 507.6 ms, System: 75.1 ms]
  Range (min … max):   530.6 ms … 542.0 ms    10 runs

(This was on a Crucial CT250MX2 SDD with a i5-2500k and 16 GiB RAM)

@benpicco benpicco requested a review from keestux as a code owner March 16, 2020 23:54
@benpicco benpicco force-pushed the cpu/sam0_common/generic_RAM_ROM branch from fa65ece to ce4cae2 Compare March 17, 2020 00:33
@benpicco
Copy link
Contributor Author

So I'd say the impact of the fully generic version is negligible.
Parsing the part number adds 8ms on average, so that's a very small sacrifice for much improved ease of use when adding new boards.

@dylad
Copy link
Member

dylad commented Mar 25, 2020

I am happy with this.
I tested benpicco PR by modifying tests/leds/Makefile as follows:

include ../Makefile.tests_common


# Some boards do not initialize LED0 by default
CFLAGS=-DAUTO_INIT_LED0

..msg:
	@echo "print CPU ROM/RAM"
	@echo CPU: $(CPU_MODEL)
	@echo ROM LENGTH: $$(($(ROM_LEN)/1024))K
	@echo RAM LENGTH: $$(($(RAM_LEN)/1024))K

print-info: ..msg

include $(RIOTBASE)/Makefile.include

then run make BOARD=samr34-xpro -C tests/leds print-info

print CPU ROM/RAM
CPU: samr34j18b
ROM LENGTH: 256K
RAM LENGTH: 32K

I tested a bunch of SAM0-based board, I also modified CPU_MODEL in some cases to check if output was changed accordingly.

@fjmolinas Are you happy with this too ?

@fjmolinas
Copy link
Contributor

@dylad you can also test with make -C info-debug-variable<VARIABLE_NAME>.

@fjmolinas Are you happy with this too ?

Performance was evaluated by @benpicco so I'm fine with this.

@dylad
Copy link
Member

dylad commented Mar 25, 2020

info-debug-variable

That's the command I wanted to use but I was unable to remember it so it was faster to rewrite something !
Thanks I guess it is time to write it down.

The ROM size is encoded in the part number of the Atmel SAM chips.
RAM size is not encoded directly, so get it by parsing the chip's vendor file.

The file remains in the page cache for the compiler to use, so the overhead
should be minimal:

on master:

  Benchmark #1: make BOARD=samr21-xpro -j
    Time (mean ± σ):     527.9 ms ±   4.9 ms    [User: 503.1 ms, System: 69.6 ms]
    Range (min … max):   519.7 ms … 537.2 ms    10 runs

with this patch:

  Benchmark #1: make BOARD=samr21-xpro -j
    Time (mean ± σ):     535.6 ms ±   4.0 ms    [User: 507.6 ms, System: 75.1 ms]
    Range (min … max):   530.6 ms … 542.0 ms    10 runs
@benpicco
Copy link
Contributor Author

Should I squash?

@dylad
Copy link
Member

dylad commented Mar 25, 2020

Should I squash?

Please, go ahead.

@benpicco benpicco force-pushed the cpu/sam0_common/generic_RAM_ROM branch from ce4cae2 to 7796110 Compare March 25, 2020 14:14
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked manually Makefile output, this looks good and buildsystem isn't impact that much so let's go.

@dylad dylad added this to the Release 2020.04 milestone Mar 31, 2020
@dylad dylad added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Mar 31, 2020
@dylad dylad merged commit 783ffdc into RIOT-OS:master Mar 31, 2020
@benpicco benpicco deleted the cpu/sam0_common/generic_RAM_ROM branch March 31, 2020 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants